-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Some fixes to #5914 noticed after merging #6013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Several minor fixes to the content in the `README.md` were pointed out in review after merging that are fixed here. I also accidentally undid the removal of the legacy strategy document. I _think_ I got all of the critical content from this incorporated into the safety design section, but if there is more that I missed, please flag and I'm happy to improve that as part of this PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It looks like the link style being used in safety/README.md isn't working on docs.carbon-lang.dev, so it might be worth looking into that too. Maybe there's just a flag we can set on the website to make it render those?
@@ -1,771 +0,0 @@ | |||
# Safety strategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this document should be deleted:
- I think we should have safety principles, in addition to a safety strategy.
- I think a lot of the content of the principles was not included in other documents as part of Updating Carbon's safety strategy #5914.
I think it would make more sense to rename this doc to "Safety principles" to clarify that it is more about tactics and approach than strategy. I think the content outside of the "principles" section can be deleted, along anything about build modes in the principles section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level, I'm happy to keep some updated form of the content you've flagged below that we shouldn't lose.
I'm not sure about keeping it under project/principles
. They don't seem to really be principles in the same sense as our other principles have ended up. They seem more like our strategy or approach for the design of safety in the language. For other major areas of our design, the high level approach description is in the design itself and not in the project principles.
Separately, I think the content of the two principles flagged (at least) would benefit from being updated to reflect the pretty significant changes in approach that have happened. I can try to do that, but I'd also be happy for someone else to work on that -- would you be interested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked to Josh and would be happy to do distill still relevant and adjusted safety principles as follow-up.
Should we file an issue, assign to me, and then move forward with deletion (I can access the content from here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #6142 for the extracted principles.
|
||
## Principles | ||
|
||
- Safety must be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I think is useful and not covered by #5914.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the updated strategy not cover this at least partially by mentioning incremental migration from C++ and the permissive vs strict distinction?
safety checks. They should also allow favor automation of testing and | ||
fuzzing. | ||
|
||
- Safety in Carbon must work with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I think is useful and not covered by #5914.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, "permissive Carbon" seems to be more specific (without being overly concrete, so choices still need to be made.)
Although some safety features will be Carbon-specific, safety should not | ||
stop at the language boundary. | ||
|
||
- The rules for determining whether code will pass compile-time safety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that is covered (to some extent) by #5914.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'd agree that this is recognizable as a principle.
I'd consider it somewhat obvious among people who work on static checking, but not everyone does so there is value in stating it explicitly.
different build modes. The purpose of the build modes is to determine | ||
code generation. | ||
|
||
- Each build mode will prioritize performance and safety differently: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that is covered (to some extent) by #5914.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider this here contradictory and would prefer there was only one place modes.
It seems to me that the spirit in which this older text was written placed more emphasis on runtime checks and various build modes, and that the direction has evolved towards pursuing rigorous memory safety.
I don't see a discussion of build modes as being an abstract principle, but rather very concrete choices. I'd wish principles (at least for safety) were short ad abstract, yet meaningfully constraining the design space.
I expect a lot of substantial design questions and decisions to juggle the multiple conflicting goals which are at play in a safety design, but I don't see a discussion of build modes as being a principle that can guide those.
binaries that prioritize safety that is resistant to attacks at the cost | ||
of performance. | ||
|
||
- Safety checks should try to be identical across build modes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that is covered (to some extent) by #5914.
- There will be differences, typically due to performance overhead and | ||
detection rate trade-offs of safety check algorithms. | ||
|
||
- The number of build modes will be limited, and should be expected to remain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that is covered (to some extent) by #5914.
Carbon developers, who can easily choose which is better for their | ||
use-case. | ||
|
||
- Each distinct safety-related build mode (debug, performance, and hardened) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true? It would be good to have the answer to this documented, though it might be better in /docs/design/safety/README.md. I do think we expect "strict" files to be able to mixed with "permissive", which may also be worth saying.
by developers interested in combining libraries built under different | ||
build modes. | ||
|
||
- Although runtime safety checks should prevent logic errors from turning into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be now covered by #5914.
For example, some safety checks would result in application termination; | ||
this prevents execution of unexpected code and still needs to be fixed. | ||
|
||
- Developers need a strong testing methodology to engineer correct software. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm neutral on whether this should be retained.
Several minor fixes to the content in the
README.md
were pointed out in review after merging that are fixed here.I also accidentally undid the removal of the legacy strategy document. I think I got all of the critical content from this incorporated into the safety design section, but if there is more that I missed, please flag and I'm happy to improve that as part of this PR as well.